harden: idempotent destroy(), sanitize wrapper classNames, partial-init safety#479
Merged
Merged
Conversation
…it safety Defense-in-depth follow-ups on the #468-#478 memory-leak/perf/security series (all validated as correct; these are non-blocking hardening): - destroy(): explicit isDestroyed guard so double-destroy and observer-vs-manual re-entrancy stay safe against future edits (was idempotent only emergently). - render()/renderDropbox(): run additionalClasses, additionalToggleButtonClasses, additionalDropboxClasses and additionalDropboxContainerClasses through Utils.sanitizeClassNames, matching the option-classNames sink (attribute-injection defense-in-depth; no change for legitimate class names). - afterRenderWrapper(): wrap post-addEvents() init in try/catch -> self-destroy on failure so a throw after registerInstance can't leak the shared observer / global listeners. - tests: add double-destroy idempotency and shared-observer auto-destroy (host removed without destroy()) coverage to observer-listener-lifecycle.cy.ts. npm run validate + build pass; full Cypress suite green (the 2 transient examples.cy.ts timeouts under load pass deterministically on isolated re-run, 215/215).
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens VirtualSelect lifecycle and rendering against double-destroy/re-entrancy, partial initialization failures, and class attribute injection, and adds e2e coverage for observer/listener lifecycle behavior.
Changes:
- Make
destroy()explicitly idempotent via anisDestroyedguard. - Sanitize the component-level
additional*Classesoptions usingUtils.sanitizeClassNames. - Wrap post-
addEvents()initialization inafterRenderWrapper()withtry/catchthatdestroy()s on failure and rethrows. - Extend Cypress lifecycle tests to cover double-destroy and shared-observer auto-destroy when a host is removed.
Reviewed changes
Copilot reviewed 3 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/virtual-select.js | Adds isDestroyed guard, sanitizes additional class options, and ensures partial-init failures self-teardown. |
| cypress/e2e/observer-listener-lifecycle.cy.ts | Adds e2e coverage for double-destroy idempotency and observer-driven auto-destroy on DOM removal. |
| dist/virtual-select.js | Generated bundle updated to reflect source changes (should not be committed in PRs per repo guidance). |
| dist/virtual-select.min.js | Generated minified bundle updated (should not be committed in PRs per repo guidance). |
| docs/assets/virtual-select.js | Generated docs bundle updated (should not be committed in PRs per repo guidance). |
| docs/assets/virtual-select.min.js | Generated minified docs bundle updated (should not be committed in PRs per repo guidance). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue number: resolves #
What is the current behavior?
The #468–#478 series fixed the reported leaks/crash/XSS correctly, but a few hardening gaps remain:
destroy()is idempotent only emergently (every step happens to be safe to repeat); there is no explicit guard, so a future non-idempotent edit — or the observer auto-destroy racing a manualdestroy()— could break.classNamesare run throughUtils.sanitizeClassNames, but the component-level class options (additionalClasses,additionalToggleButtonClasses,additionalDropboxClasses,additionalDropboxContainerClasses) are interpolated intoclass="…"without sanitization.afterRenderWrapper(),addEvents()(which callsregisterInstanceand installs the shared observer + page-level listeners) runs beforesetEleProps()/initDropboxPopover()/setValueMethod(). If one of those throws, the instance is registered but the caller has no handle todestroy()it — leaking the global listeners/observer.destroy()or the shared observer's primary job (auto-destroying an instance whose host is removed from the DOM without callingdestroy()).What is the new behavior?
destroy(): explicitisDestroyedearly-return guard (set once in the constructor) so double-destroy and observer-vs-manual re-entrancy stay safe regardless of future edits.render()/renderDropbox(): the fouradditional*Classesoptions now pass throughUtils.sanitizeClassNames, matching the option-classNamessink. Legitimate class names are unchanged (only",<,>are stripped).afterRenderWrapper(): post-addEvents()initialization is wrapped intry/catchthat callsthis.destroy()and rethrows, so a failure afterregisterInstancecan no longer leak the shared observer / global listeners. The success path is behaviorally identical.observer-listener-lifecycle.cy.tsgains (1) a double-destroy()idempotency test and (2) a test asserting the shared observer auto-destroys an instance when its host node is removed withoutdestroy()(back-reference cleared,activeInstancesempty,hasGlobalListeners === false,domObserver === null).Does this introduce a breaking change?
Other information
npm run validate(tsc strict + eslint + stylelint) andnpm run buildpass;dist/,docs/assets/, anddist-archive/bundles were regenerated to stay in sync with source.examples.cy.tspasses 215/215 on an isolated run (two transient timeouts seen only under a resource-contended full run are pre-existing focus/visibility flakiness on code paths untouched here).